Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #184 Fix Size calculations Fix Stream Size validations #189

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

ironfede
Copy link
Owner

Fix Size calculations
Fix Stream Size validations

Fix Size calculations
Fix Stream Size validations
@ironfede
Copy link
Owner Author

ironfede commented Oct 10, 2024

@jeremy-visionaid could you please take a look to this PR? I've used your validations as a guide for a code refactoring/fixing of stream sizes and calculation. This should make things more solid and predictables in stream size validations and calculations. Now all test passes (no change in test files required). BTW I've also found some errors that were masked and avoided by self adaption of stream-view in adjusting its size... Many thanks!

P.S.
Obviously review and comments are welcome from everyone interested!

@jeremy-visionaid
Copy link
Contributor

@ironfede LGTM, though I'm currently no expert in CFB. I think we should also take #187 and #188 for belt and braces. I'm happy to rebase them on top of this if you want to merge this one first.

@ironfede
Copy link
Owner Author

@ironfede LGTM, though I'm currently no expert in CFB. I think we should also take #187 and #188 for belt and braces. I'm happy to rebase them on top of this if you want to merge this one first.

Perfect! I'm going to merge first since changes are a little bit "deeper" in the core of size calculations so we can check after that if your new tests still works ok!

Many thx

@ironfede ironfede merged commit ceb409e into master Oct 10, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants